@eugeneia @lukego I agree with Luke's point of view, the snippet of code is run every time configure is called so better call the event configure instead of start. However that's not what I originally intended. Originally the trigger of the event was from ops:start(), but when doing it from there the links are not set up at that point, so that makes start less useful. So I moved the code to after the links are setup, but that changes the semantic of the event as the code is called every time.

I'm OK with calling it configure, but as apps can implement stop I think having a start counterpart makes sense. If going for this solution, the start loop should only be called if ops.start was executed.

Ugh I fat-fingered the close button, sorry!

@dpino I would say new is the counterpart to stop. I think configure makes sense, as this callback lets apps react to the way they were configured.

@eugeneia Taking a look at Alexander's patch I think it makes sense to call it configure too, as he originally called that method post_config. Agree with the name change.

Thanks for the name change!

I would still like to address these issues before merge:

• Exactly when is configure called and what are the intended uses? Seems to me like the implementation is a hook for every time engine.configure() is called independent of the content of the configuration changes and that seems potentially excessive to me (more often than I would expect from reading the documentation).
• Do we really want to have both configure and reconfig callbacks? This is the situation on the submitted branch. I think we should either merge these together or otherwise clearly explain when to use one vs the other. (If we keep both then we should also be consistent about abbreviating their names or not i.e. config vs configure.)

I would like to please withdraw my objection to the implementation of configure.

I checked how the relink() callback had been implemented (added in commit 4f166b3 and removed in commit c4a7885) and it is exactly the same. Could be seen as premature optimization to make a more complex implementation that runs the callback less frequently e.g. skipping for apps whose own links and configurations have not been touched.

So my only request now is to please reconcile configure with reconfig either by merging them together somehow or making their names/purposes more clearly distinct :).

I think I can explain best what I need if I describe the use cases I'm trying to fix.

I would like to give apps a chance to complete their initialization once they have been configured (with their links setup too). app:reconfig is only called if an app already exists in the app_array so it cannot be used.

One example that could take advantage of this new event is the NDP app in the lwAFTR.

https://github.com/Igalia/snabb/blob/lwaftr/src/apps/lwaftr/ipv6_apps.lua#L186

There's a case where the NDP app needs to send Neighbour Solicitation packets until a parameter is resolved (self.dst_eth). At this moment, we are implementing this logic in the push method, because only at that point links are setup. However, we could start sending NS packets as soon as the app and the links are ready.

It could be argued that the links won't be processed until push is called in any case, but putting this logic in two different methods would make the code cleaner and the push method less bloated IMHO.

Another use case is creating counters by app name. An app name is added to an app on start, but since there's no start callback apps can react to, the app has to wait until push to create its counters and it has to do it only once. Something like:

function App:push ()
    if not self.added_counters then
       self.counter = counter.open(self.appname.."/counter")
       self.added_counters = true
   end
   local input, output = assert(self.input.input), assert(self.output.output)
   while not link.empty(input) do
      ...
   end
end

I think a method where apps could further initialize themselves (in new appname or links do not exist), seems necessary.

Solutions I can think of:

• We could discard this new configure method and call reconfig on start too, but that would make the reconfig and startstates less clear. In addition to that, the first time reconfig is called, links wouldn't exist. I doesn't seem like a good solution :(
• I go back to my original request of adding a start event (I agree having a configure and reconfig callbacks would be confusing). I wanted to defer the call of the event until the links were created, but I didn't do it right. Should be something like:

function apply_config_actions (actions, conf)
   ...
   local started_apps = {}
   function ops.start (name)
      ...
      table.insert(started_apps, app)
   end
   -- Setup links.
   ...
   for _, app in ipairs(started_apps) do
      if app.start then app:start() end
   end
end

@diego Thanks for providing that context. That helps a lot.

How about implementing this all locally in the push method like this?

function NDP:push ()
   self:maybe_send_ns()
   ... process packets ...
end

-- Send rate-limited neighbor solicitation message.
function NDP:maybe_send_ns ()
   self.next_ns_time = self.next_ns_time or engine.now() -- first message is immediate
   if self.next_ns_time <= engine.now() then
      self:send_ns()
      self.next_ns_time = engine.now() + self.ns_interval
   end
end

This seems neat, predictable, and non-bloated to me. The send_ns() function will be called at regular intervals and take the appropriate (non-)action based on the current links and configuration. How about it?

Note that lib.timer should not be used from apps at all and this seems to be the root of our difficulty here. Those timers are persistent, like crontab entries, and can easily drift out of sync with the app's own lifetime and configuration. You don't want zombie timers accumulating and running in the background and referencing obsolete objects via closures. I suspect that the root of our whole difficulty here is trying to match the lifecycles of apps and lib.timer timers which don't align very well. (Maybe we should rename lib.timer to lib.cron to make its appropriate usage more obvious?)

WDYT?

Incidentally I would quite like to have a tick() timer in apps that runs at a fixed frequency e.g. 1 millisecond. This could be used for taking care of administrivia such as checking whether it is time to send a neighbor solicitation message. This is something we could add if it would simplify or speed up existing apps in practice.

@lukego Thanks for the detailed explanation, I like this new design more. In fact, before I needed to add a timer.deactivate method which I can remove now. I was not aware that apps should not depend on timers to implement their logic.

@lukego Last commit renames and documents the callback as per #946.

Merged. Thanks and sorry about the wait!